Spine correctness, hardening and poll_host decomposition - #597

php_processes, debug_devices and the two connection pools were dereferenced
on the next line without testing the allocation.

Closes Cacti#564

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The two strncat() calls above it may fill flogmessage exactly, so the
unconditional strcat() put the terminator one byte past a LOGSIZE buffer.

Closes Cacti#565

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Closes Cacti#566

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The format depends only on two settings read in read_config_options(), so
rebuilding it per log line cost a malloc/free pair and two switches.

Closes Cacti#567

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Adds a suite that includes util.c the way test_util_strings.c does, so the
date format caching, the settings-helper result release and the bounded
newline are exercised as shipped rather than as copies, plus an LD_PRELOAD
allocation-failure test for the two startup guards no unit test can reach.

test_linked freed the result of get_date_format() and expected that call to
clamp; both moved to set_date_format() with this change, so its assertions
move with them.

Changed-line coverage on this branch goes from 38% to 79%; the remainder
needs a live database and is noted in the pull request.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

db_escape() staged its input through a fixed DBL_BUFSIZE buffer before
escaping, so every caller was capped at 1022 input bytes regardless of the
destination it passed. A poller result of 1024 bytes or more was silently cut,
which matters because RESULTS_BUFFER defaults to 2048 and is tunable through
--with-results-buffer precisely to carry multi-value output. A cut result loses
its trailing field:value pair, so that data source gets a bad RRD update every
cycle.

The staging buffer also made the bound arithmetic wrong for a small
destination: with max_size 1, (trim_limit / 2) - 1 is -1, which snprintf takes
as an unbounded size, and the escaped result was then written into a
destination far smaller than it. Building the new tests against the old
implementation reproduces that as a glibc 'buffer overflow detected' abort.

Derive the limit from the destination instead, and escape straight from the
caller's input. poller.c sizes escaped_result at 2N+1 for its RESULTS_BUFFER
source, which is what the doubling requires.

Six cases added to test_linked, run against the shipped sql.o: metacharacter
escaping, a NULL input, a full RESULTS_BUFFER round trip, the 1022 to 2047
boundary the old staging buffer cut at, truncation into a destination too small
to hold the escaped form, and a degenerate max_size of 0 and 1.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Cacti stores the literal [None] when no SNMPv3 protocol is selected, and an
empty string for an absent passphrase. Since Cacti#373 spine treated both as errors,
so two of the three security levels stopped working while cmd.php kept
accepting them.

usm_lookup_auth_type([None]) returns -1, and the lookup returned 0 from the
session setup on any non-positive result, so a noAuthNoPriv device never opened
a session. Separately the authentication key was derived only on the privacy
path, so an authNoPriv session authenticated with securityAuthKeyLen still 0
and the agent answered with a USM authentication failure, which points the
operator at their credentials rather than at spine.

Decide the level first, from the four values Cacti supplies, and only require a
recognised protocol when authentication is actually in use. Derive the key
whenever the level authenticates rather than only when it also encrypts.

spine_snmpv3_security_level() is pure and covered by five cases in test_linked:
the [None] sentinel, each level, a protocol without its passphrase, and privacy
selected without authentication, which SNMPv3 does not offer and which must not
raise the level.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

snprintf returns the length it would have written, so the cursor moves past
the buffer on the first truncation and the next remainder underflows to a
huge size_t: an effectively unbounded size at a destination already out of
bounds.

util.c:829-851 was the worst of it. Every call there passed a fixed BUFSIZE
rather than a shrinking remainder, so the bound never moved, and one of them
interpolates set.host_id_list, which --hostlist can fill with 65534 bytes.

spine_appendf() owns the arithmetic and reports truncation. Converted all 53
sites, keeping each recompute line so no buffer depends on being initialised
in a new place.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

poller.c is the most frequently changed file in the tree and validate_result()
is where it decides whether a polled value is stored at all, yet neither it nor
is_multipart_output() had any coverage. A mistake in either silently discards
data or accepts a malformed multi-value string, and nothing would fail.

Six cases added to test_linked, so they run against the real poller.o rather
than a copy: numeric forms, null and junk, single and multi field output, the
space-to-delimiter balance that separates real multi-value output from prose
containing a colon, and the trim contract.

The trim case documents an asymmetry worth knowing. rtrim() writes a NUL over
the trailing run so it edits the caller's buffer, while ltrim() only advances a
pointer, so the leading run survives in the buffer the caller keeps. The trim
set also covers quotes and a backslash, not just whitespace. Written against
what the code does, after a first draft asserting the tidier behaviour failed.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Every build job repeated the same apt-get install and bootstrap/configure
pair, so a new dependency meant editing five places. The setup now lives in
.github/actions/build-spine.

mysql-server is dropped from the package set: configure only looks for
mysql.h and libmysqlclient, both from libmysqlclient-dev, and no job starts
or connects to a server.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The sanitizer build only ran --version and --help, which exercises almost
none of the poller. make check is what drives the code, so it is what the
sanitizers should watch.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

poll_host() is 1,926 lines and builds the same six queries twice, once for the
main poller and once for a remote one. The bodies differ by 33 of 141 lines,
and every difference is the same rule: the main poller reads items that are not
deleted, a remote poller reads the items assigned to it.

Nothing in that construction was reachable from a test, so a column added to
one copy and not the other would not have been caught.

Extract the rule as poller_item_scope() and poller_owner_scope(), covered by
five cases in test_linked against the shipped poller.o: the deleted filter on
the main poller, the ownership filter on a remote one, the empty fragment the
main poller needs so callers can interpolate unconditionally, and a degenerate
buffer.

No call site changes yet. Collapsing the two branches onto these helpers is the
next step and is worth its own review, because the shipped query1 spells its
tail 'poller_id=%i' while the others spell it 'poller_id = %i', so the unified
text will normalise that.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The two branches drifted: the remote copy never picked up the dbonupdate
handling the main copy grew, and nothing could have caught that because the
construction was not reachable from a test.

Captured what both branches emit across the vectors they switch on, collapsed
them onto poller_item_scope()/poller_owner_scope(), and diffed. Output is
byte-identical except query1 on a remote poller, which now spells its tail
'poller_id = N' rather than 'poller_id=N' to match the other five queries.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

set.dbonupdate is 1 on MySQL 8, which deprecated VALUES() in ON DUPLICATE KEY
UPDATE. The main poller switched to the row-alias form; the remote branch kept
the deprecated one because it had its own copy of the suffix. Closes Cacti#590.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The construction was 167 lines in the middle of a 1,795-line function, so
nothing could reach it. It now takes its inputs as arguments and fills a
struct, which is what lets the golden capture become a test that runs instead
of a file with instructions attached.

poll_host is 1,620 lines, down from 1,927 on develop.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

poll_host() leaves through three places and each spelled its teardown out
again. The copies were not in the same order and did not hold the same steps:
the device-row-missing path never called mysql_thread_end(), so a device
deleted mid-cycle leaked the client library's thread-local state once per
cycle. Closes Cacti#594.

The two early exits now share poll_host_release(). The normal exit uses
error_string after the point where that helper frees it, so it shares only
poll_host_release_connections() and keeps its own ordering.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The assert was written out three times inside poll_host(), once per operator,
each with the same twenty-four lines of logging and reindex queueing around a
one-line comparison. The three copies of the REPLACE INTO had drifted to three
different spellings of the same SQL.

Two details were buried in there and are now stated and covered: '=' compares
as text while '<' and '>' compare as numbers, and an assert_value of '0' never
fails a '<' assert, which is what stops a device with no recorded uptime from
looking like it rebooted.

The chain is 34 lines instead of 88. poll_host is 1,516 lines, down from 1,927
on develop.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Each branch appended to the same registration array, so every pair of them
conflicted there. Reconciled once: the additions from all seven, the
get_date_format changes from the log work, and the two tests it dropped
because that change makes get_date_format return storage it does not own.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>